Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Start Surface Distance Library #26933

Merged
merged 2 commits into from
May 27, 2024
Merged

Conversation

MattKear
Copy link
Contributor

Following on from the discussion in this PR: #26827, it was agreed that the best way forward was to move the copter rangefinder functionality into a library (we called it AP_HAGL then). This PR is the start of that process.

No functional change should result from this PR

As his way, @IamPete1 already had a branch for it! #20535

I have rebased his original PR to get this. I will continue the work and do the testing.

@rmackay9
Copy link
Contributor

rmackay9 commented May 1, 2024

This is looking good! Let's call the new library "AP_SurfaceDistance" because we will likely use it in Plane as well.

@tridge
Copy link
Contributor

tridge commented May 1, 2024

maybe a SURF log message? with instance number

@tridge tridge removed the DevCallEU label May 1, 2024
@MattKear
Copy link
Contributor Author

MattKear commented May 1, 2024

Key things to test are:

  • Behaviour with rangefinder out of range
  • Behaviour with rangefinder enable/disable on RC switch

@MattKear MattKear force-pushed the surface_distance_lib branch 2 times, most recently from 3bee0d2 to cceaf50 Compare May 1, 2024 12:52
@MattKear
Copy link
Contributor Author

MattKear commented May 1, 2024

Renamed to AP_SurfaceDistance and added logging. For logging I have added a status bitmask which should make it easier to track various fault conditions.

@timtuxworth
Copy link
Contributor

This is looking good! Let's call the new library "AP_SurfaceDistance" because we will likely use it in Plane as well.

Definitely want this in Plane!

ArduCopter/Copter.h Show resolved Hide resolved
libraries/AP_SurfaceDistance/AP_SurfaceDistance.cpp Outdated Show resolved Hide resolved
libraries/AP_SurfaceDistance/AP_SurfaceDistance.cpp Outdated Show resolved Hide resolved
libraries/AP_SurfaceDistance/AP_SurfaceDistance.cpp Outdated Show resolved Hide resolved
libraries/AP_SurfaceDistance/AP_SurfaceDistance.h Outdated Show resolved Hide resolved
libraries/AP_SurfaceDistance/AP_SurfaceDistance.h Outdated Show resolved Hide resolved
libraries/AP_SurfaceDistance/AP_SurfaceDistance.cpp Outdated Show resolved Hide resolved
@clydemcqueen
Copy link
Contributor

This is great work! I would love to see sub use this in a future PR.

@MattKear
Copy link
Contributor Author

I have flight tested this code (git hash: 426c948) All appears to working as before.

Here is a break down of the test:
image

log file here: https://drive.google.com/file/d/1myPkiXPjzz0Ay9KEJwMSnXHhpa4FlaGW/view?usp=sharing

@MattKear
Copy link
Contributor Author

image
Made those changes and tested logging IRL in the office.
Only had a downward facing LiDaR fitted so only the first instance of SURF is populated with data in this test.

@MattKear
Copy link
Contributor Author

Apologies, I cannot make the call this evening, but this PR has been through a few reviews now and I am hopeful that it is good to go.
As a reminder, since last review the following changes have been made:

  • Rename RANGEFINDER_HEALTH_MAX to RANGEFINDER_HEALTH_MIN
  • Logging has been changed to report in meters and fixed-up data types. This has been tested. See plot above.
  • Semaphore added to avoid accessing last_healthy_ms out of sequence with millis() in data_stale() helper.

Copy link
Contributor

@rmackay9 rmackay9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to see the testing results! Thanks!

@tridge tridge merged commit ae38c96 into ArduPilot:master May 27, 2024
91 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants